Huiswerk #1
Huiswerk #1
Conversation
| .image-wrapper { | ||
| display: flex; | ||
| align-items: center; | ||
| gap: 10px; |
There was a problem hiding this comment.
In de uitwerkingen wordt hier een relatieve maat (1rem) gebruikt, maar volgens mij hadden we die destijds nog niet behandeld (had het zelf ook niet).
| gap: 10px; | ||
| align-items: center; | ||
| list-style: none; | ||
| } |
There was a problem hiding this comment.
Ook hier kan een relatieve gap-maat dus ook, en list-style is in de uitwerkingen in een aparte class selector gezet die globaal wordt toegepast. Maar dit is simpelweg een andere manier om het goed te doen lijkt me.
| .navbar-four { | ||
| display: flex; | ||
| justify-content: space-between; | ||
| } |
There was a problem hiding this comment.
Er is in deze 4e subopdracht misschien wat weinig ruimte tussen 'Novi Futuristic Inc' en 'Home', omdat ze allebei bij een andere 'groep' horen had je hier nog een gap kunnen aanmaken voor meer ruimte (2rem in de uitwerkingen).
| </div> | ||
| <div class="menu"> | ||
| <div> | ||
| <ul> |
There was a problem hiding this comment.
In vergelijking met de uitwerkingen ben je hier 2 div's dieper gegaan, in plaats van het gebruik van een class voor de
- (en een class voor de meld je aan/login knoppen, die ook als
- hadden kunnen worden samengepakt). Visueel gezien bereik je dus hetzelfde resultaat met iets meer code.
| flex: 1 0 250px; | ||
| flex-direction: column; | ||
| gap: 20px; | ||
| } |
There was a problem hiding this comment.
Zelfde als bij eerste deel opdracht, absolute px-waarden ipv relatieve rem-waarden.
| gap: 24px; | ||
| justify-content: center; | ||
| padding: 24px; | ||
| } |
There was a problem hiding this comment.
De uitwerkingen kiezen hier voor alleen een overflow-waarde op de y-as (overflow-y: scroll;).
| article { | ||
| display: flex; | ||
| max-width: 260px; | ||
| max-height: 500px; |
There was a problem hiding this comment.
In plaats van absolute afmetingen geeft de uitwerkingen een 'aspect-ratio' attribuut terug, die ik zelf ook nog niet kende (aspect-ratio: 1 / 1.4;). Zodat het meeschaalt afhankelijk van de content maar wel in dezelfde proporties blijft.
| } | ||
|
|
||
| .icon { | ||
| max-width: 16px; |
There was a problem hiding this comment.
Width of max-width lijkt hier tot hetzelfde resultaat te leiden.
ltjansen
left a comment
ltjansen
left a comment
There was a problem hiding this comment.
Hi Dennis, ik heb m'n best gedaan om je code te reviewen. Ziet er prima uit, zelfs een stukje efficiënter dan de uitwerkingen hier en daar zo lijkt het!
Los van de paar losse comments die ik hier en daar onder code heb gezet valt het me op dat je vaker voor een (combinatie van) type selectors bent gegaan dan voor class selectors. Op basis van de uitwerkingen lijkt het dat er een voorkeur is voor gebruik van class selectors, maar dat zal vooral voordelig gaan zijn bij grotere projecten denk ik (en hier was het nog niet noodzakelijk).
Hopelijk heb je iets aan deze feedback, als je nog specifieke feedback mist of als ik iets onduidelijk heb opgeschreven, laat het me vooral weten!
Cheers, Lukas
No description provided.